diff mbox series

[BlueZ] adapter: Include pending flags in device_flags_changed_callback

Message ID 20250128102937.16215-1-ludovico.denittis@collabora.com (mailing list archive)
State New
Headers show
Series [BlueZ] adapter: Include pending flags in device_flags_changed_callback | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Ludovico de Nittis Jan. 28, 2025, 10:29 a.m. UTC 
When signalling the new device flags, we need to include the pending
ones. Otherwise, the eventual non-zero `pending_flags` will be wiped out
in `btd_device_flags_changed()`, and we'll lose the pending changed
flags.

Fixes https://github.com/bluez/bluez/issues/1076
---
 src/adapter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

bluez.test.bot@gmail.com Jan. 28, 2025, 11:32 a.m. UTC | #1 
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=928792

---Test result---

Test Summary:
CheckPatch                    PENDING   0.20 seconds
GitLint                       PENDING   0.17 seconds
BuildEll                      PASS      20.12 seconds
BluezMake                     PASS      1456.52 seconds
MakeCheck                     PASS      13.66 seconds
MakeDistcheck                 PASS      156.54 seconds
CheckValgrind                 PASS      217.32 seconds
CheckSmatch                   PASS      271.38 seconds
bluezmakeextell               PASS      98.09 seconds
IncrementalBuild              PENDING   0.25 seconds
ScanBuild                     PASS      850.71 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Jan. 28, 2025, 8:33 p.m. UTC | #2 
Hi Ludovico,

On Tue, Jan 28, 2025 at 5:30 AM Ludovico de Nittis
<ludovico.denittis@collabora.com> wrote:
>
> When signalling the new device flags, we need to include the pending
> ones. Otherwise, the eventual non-zero `pending_flags` will be wiped out
> in `btd_device_flags_changed()`, and we'll lose the pending changed
> flags.
>
> Fixes https://github.com/bluez/bluez/issues/1076
> ---
>  src/adapter.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 5d4117a49..cbc0f678f 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5727,6 +5727,7 @@ static void device_flags_changed_callback(uint16_t index, uint16_t length,
>         struct btd_adapter *adapter = user_data;
>         struct btd_device *dev;
>         char addr[18];
> +       uint32_t changed_flags = 0;
>
>         if (length < sizeof(*ev)) {
>                 btd_error(adapter->dev_id,
> @@ -5744,7 +5745,9 @@ static void device_flags_changed_callback(uint16_t index, uint16_t length,
>                 return;
>         }
>
> -       btd_device_flags_changed(dev, ev->supported_flags, ev->current_flags);
> +       changed_flags = ev->current_flags | btd_device_get_pending_flags(dev);
> +
> +       btd_device_flags_changed(dev, ev->supported_flags, changed_flags);

This doesn't sound right, it would be as if the pending flags always
succeed which I don't think it is always the case, perhaps we should
something like the following:

diff --git a/src/device.c b/src/device.c
index e8bff718c201..4959f36542fb 100644
--- a/src/device.c
+++ b/src/device.c
@@ -7413,7 +7413,7 @@ void btd_device_flags_changed(struct btd_device
*dev, uint32_t supported_flags,

        dev->supported_flags = supported_flags;
        dev->current_flags = current_flags;
-       dev->pending_flags = 0;
+       dev->pending_flags &= ~changed_flags;

        if (!changed_flags)
                return;

That way we clear pending_flags based on the changed_flags.

>  }
>
>
> --
> 2.48.1
>
>
Ludovico de Nittis Jan. 29, 2025, 5:25 p.m. UTC | #3 
Right, makes sense. I did something similar to what you suggested in 
"[PATCH BlueZ] device: Clear only the pending flags that have been applied".

However, I changed it to `dev->pending_flags &= ~current_flags`, because 
otherwise we could have left `pending_flags` to some unexpected value.

For example, if we are in `device_set_wake_allowed()` and we want to 
enable the wake allowed:
- Assume the current flags is 2, i.e. DEVICE_FLAG_DEVICE_PRIVACY
- It calls `adapter_set_device_flags()` with current flags plus 
DEVICE_FLAG_REMOTE_WAKEUP (aka 3)
- In `adapter_set_device_flags()` we have `flags == 3`, which then gets 
assigned to `pending_flags`
- In `btd_device_flags_changed()` we end up with `changed_flags == 1`

In this situation, if we only remove the `changed_flags`, pending flags 
will remain with a value of 2 even if there are no other pending changes.
For this reason I remove the current_flags, instead of only changed_flags.

The other two patches that I created "[PATCH BlueZ v2] device: Clear 
pending_flags on error" and "[PATCH BlueZ] adapter: Fix the pending 
changing flags check", are kinda related to this one, but I sent them 
separately because they don't really depend on one another.


On 1/28/25 9:33 PM, Luiz Augusto von Dentz wrote:
> Hi Ludovico,
>
> On Tue, Jan 28, 2025 at 5:30 AM Ludovico de Nittis
> <ludovico.denittis@collabora.com> wrote:
>> When signalling the new device flags, we need to include the pending
>> ones. Otherwise, the eventual non-zero `pending_flags` will be wiped out
>> in `btd_device_flags_changed()`, and we'll lose the pending changed
>> flags.
>>
>> Fixes https://github.com/bluez/bluez/issues/1076
>> ---
>>   src/adapter.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 5d4117a49..cbc0f678f 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -5727,6 +5727,7 @@ static void device_flags_changed_callback(uint16_t index, uint16_t length,
>>          struct btd_adapter *adapter = user_data;
>>          struct btd_device *dev;
>>          char addr[18];
>> +       uint32_t changed_flags = 0;
>>
>>          if (length < sizeof(*ev)) {
>>                  btd_error(adapter->dev_id,
>> @@ -5744,7 +5745,9 @@ static void device_flags_changed_callback(uint16_t index, uint16_t length,
>>                  return;
>>          }
>>
>> -       btd_device_flags_changed(dev, ev->supported_flags, ev->current_flags);
>> +       changed_flags = ev->current_flags | btd_device_get_pending_flags(dev);
>> +
>> +       btd_device_flags_changed(dev, ev->supported_flags, changed_flags);
> This doesn't sound right, it would be as if the pending flags always
> succeed which I don't think it is always the case, perhaps we should
> something like the following:
>
> diff --git a/src/device.c b/src/device.c
> index e8bff718c201..4959f36542fb 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -7413,7 +7413,7 @@ void btd_device_flags_changed(struct btd_device
> *dev, uint32_t supported_flags,
>
>          dev->supported_flags = supported_flags;
>          dev->current_flags = current_flags;
> -       dev->pending_flags = 0;
> +       dev->pending_flags &= ~changed_flags;
>
>          if (!changed_flags)
>                  return;
>
> That way we clear pending_flags based on the changed_flags.
>
>>   }
>>
>>
>> --
>> 2.48.1
>>
>>
>
diff mbox series

Patch

diff --git a/src/adapter.c b/src/adapter.c
index 5d4117a49..cbc0f678f 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -5727,6 +5727,7 @@  static void device_flags_changed_callback(uint16_t index, uint16_t length,
 	struct btd_adapter *adapter = user_data;
 	struct btd_device *dev;
 	char addr[18];
+	uint32_t changed_flags = 0;
 
 	if (length < sizeof(*ev)) {
 		btd_error(adapter->dev_id,
@@ -5744,7 +5745,9 @@  static void device_flags_changed_callback(uint16_t index, uint16_t length,
 		return;
 	}
 
-	btd_device_flags_changed(dev, ev->supported_flags, ev->current_flags);
+	changed_flags = ev->current_flags | btd_device_get_pending_flags(dev);
+
+	btd_device_flags_changed(dev, ev->supported_flags, changed_flags);
 }