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