[Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Archie Pusaka]

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);
+	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);
 
@@ -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

RE: [Bluez] adapter: Reset pending settings when receiving MGMT error

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=669623

---Test result---

Test Summary:
CheckPatch                    PASS      0.88 seconds
GitLint                       PASS      0.57 seconds
Prep - Setup ELL              PASS      26.91 seconds
Build - Prep                  PASS      0.63 seconds
Build - Configure             PASS      8.33 seconds
Build - Make                  PASS      935.72 seconds
Make Check                    PASS      11.03 seconds
Make Check w/Valgrind         PASS      270.28 seconds
Make Distcheck                PASS      226.70 seconds
Build w/ext ELL - Configure   PASS      8.24 seconds
Build w/ext ELL - Make        PASS      79.93 seconds
Incremental Build w/ patches  PASS      0.00 seconds
Scan Build                    PASS      532.54 seconds



---
Regards,
Linux Bluetooth

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Paul Menzel]

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.

> 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?

> Therefore, also clear the pending flag when receiving error.
> Furthermore, this patch also postpone setting the pending flag

postpone*s*

> until we queue the MGMT command in order to avoid setting it too
> soon but we return early.

Maybe add a comment, that how you tested this?

> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> 
> ---
> 
>   src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 37 insertions(+), 8 deletions(-)

[…]


Kind regards,

Paul

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Archie Pusaka]

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.
>
> > Therefore, also clear the pending flag when receiving error.
> > Furthermore, this patch also postpone setting the pending flag
>
> postpone*s*

Thanks, will fix.
>
> > until we queue the MGMT command in order to avoid setting it too
> > soon but we return early.
>
> Maybe add a comment, that how you tested this?

The reporter claims the problem is no longer observable after this
patch. I didn't do any other intelligent way of testing,
unfortunately. Do I also need to document that?
>
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> >
> > ---
> >
> >   src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 37 insertions(+), 8 deletions(-)
>
> […]
>
>
> Kind regards,
>
> Paul

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Paul Menzel]

Dear Archie,


Am 22.08.22 um 08:33 schrieb Archie Pusaka:

> On Mon, 22 Aug 2022 at 14:15, Paul Menzel <pmenzel@molgen.mpg.de> wrote:

>> 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.

Yes, I noticed after hitting *Send*.

>>> 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.
>>
>>> Therefore, also clear the pending flag when receiving error.
>>> Furthermore, this patch also postpone setting the pending flag
>>
>> postpone*s*
> 
> Thanks, will fix.
>>
>>> until we queue the MGMT command in order to avoid setting it too
>>> soon but we return early.
>>
>> Maybe add a comment, that how you tested this?
> 
> The reporter claims the problem is no longer observable after this
> patch. I didn't do any other intelligent way of testing,
> unfortunately. Do I also need to document that?

Is the bug report public.

It’s not a requirement. I just thought, that the Chromium project has a 
big QA setup, and runs on millions of devices, it’d be good to know, for 
example, if the patch was battle proven for several months in production 
or if it’s verified by one person.


Kind regards,

Paul


>>
>>> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
>>>
>>> ---
>>>
>>>    src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
>>>    1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> […]

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Archie Pusaka]

Hi Paul,

On Mon, 22 Aug 2022 at 14:40, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Archie,
>
>
> Am 22.08.22 um 08:33 schrieb Archie Pusaka:
>
> > On Mon, 22 Aug 2022 at 14:15, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> >> 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.
>
> Yes, I noticed after hitting *Send*.
>
> >>> 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.
> >>
> >>> Therefore, also clear the pending flag when receiving error.
> >>> Furthermore, this patch also postpone setting the pending flag
> >>
> >> postpone*s*
> >
> > Thanks, will fix.
> >>
> >>> until we queue the MGMT command in order to avoid setting it too
> >>> soon but we return early.
> >>
> >> Maybe add a comment, that how you tested this?
> >
> > The reporter claims the problem is no longer observable after this
> > patch. I didn't do any other intelligent way of testing,
> > unfortunately. Do I also need to document that?
>
> Is the bug report public.

No, the bug report is filed by Vendor testing unreleased devices, so
unfortunately it is not public.
>
> It’s not a requirement. I just thought, that the Chromium project has a
> big QA setup, and runs on millions of devices, it’d be good to know, for
> example, if the patch was battle proven for several months in production
> or if it’s verified by one person.

Chromium usually holds the "upstream first" spirit, this patch is no
exception. So, currently it is not battle proven.
Whether accepted or not, we would still merge it to the Chromium tree
though. If required, by that time I can circle back to this patch and
report any future findings.
>
>
> Kind regards,
>
> Paul
>
>
> >>
> >>> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> >>>
> >>> ---
> >>>
> >>>    src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
> >>>    1 file changed, 37 insertions(+), 8 deletions(-)
> >>
> >> […]

Thanks,
Archie

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Paul Menzel]

Dear Archie,


Am 22.08.22 um 08:49 schrieb Archie Pusaka:

> On Mon, 22 Aug 2022 at 14:40, Paul Menzel <pmenzel@molgen.mpg.de> wrote:

>> Am 22.08.22 um 08:33 schrieb Archie Pusaka:
>>
>>> On Mon, 22 Aug 2022 at 14:15, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>>
>>>> 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.
>>
>> Yes, I noticed after hitting *Send*.
>>
>>>>> 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.
>>>>
>>>>> Therefore, also clear the pending flag when receiving error.
>>>>> Furthermore, this patch also postpone setting the pending flag
>>>>
>>>> postpone*s*
>>>
>>> Thanks, will fix.
>>>>
>>>>> until we queue the MGMT command in order to avoid setting it too
>>>>> soon but we return early.
>>>>
>>>> Maybe add a comment, that how you tested this?
>>>
>>> The reporter claims the problem is no longer observable after this
>>> patch. I didn't do any other intelligent way of testing,
>>> unfortunately. Do I also need to document that?
>>
>> Is the bug report public.
> 
> No, the bug report is filed by Vendor testing unreleased devices, so
> unfortunately it is not public.

Understood. For others to reproduce later on, I think, it’s always good 
to have some hints, on what to do. Especially with Bluetooth where two 
devices are involved. Maybe share as much information as you can. (But, 
it’s also my personal opinion. It’s not required.)
>> It’s not a requirement. I just thought, that the Chromium project has a
>> big QA setup, and runs on millions of devices, it’d be good to know, for
>> example, if the patch was battle proven for several months in production
>> or if it’s verified by one person.
> 
> Chromium usually holds the "upstream first" spirit, this patch is no
> exception. So, currently it is not battle proven.
> Whether accepted or not, we would still merge it to the Chromium tree
> though. If required, by that time I can circle back to this patch and
> report any future findings.

Ah, then I made the wrong assumptions from the address 
chromeos-bluetooth-upstreaming@chromium.org. Sorry about that, and thank 
you for clearing that up. I am going to remember that.


Kind regards,

Paul

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Luiz Augusto von Dentz]

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.

> +       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.

> @@ -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

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Luiz Augusto von Dentz]

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.

> >
> > > Therefore, also clear the pending flag when receiving error.
> > > Furthermore, this patch also postpone setting the pending flag
> >
> > postpone*s*
>
> Thanks, will fix.
> >
> > > until we queue the MGMT command in order to avoid setting it too
> > > soon but we return early.
> >
> > Maybe add a comment, that how you tested this?
>
> The reporter claims the problem is no longer observable after this
> patch. I didn't do any other intelligent way of testing,
> unfortunately. Do I also need to document that?
> >
> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > >
> > > ---
> > >
> > >   src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 37 insertions(+), 8 deletions(-)
> >
> > […]
> >
> >
> > Kind regards,
> >
> > Paul



-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error

[Archie Pusaka]

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